fix(filterFilter): correctly handle deep expression objects by gkalpak · Pull Request #9757 · angular/angular.js

gkalpak · 2014-10-23T10:19:15Z

Previously, trying to use a deep expression object (i.e. an object whose properties can be objects themselves) did not work correctly. This commit refactors filterFilter, making it simpler and adding support for filtering collections of arbitrarily deep objects.

Closes #9698

_{BTW, I used "non-IE8" stuff (like `Array.prototype.some/every`) for the sake of conciseness and clarity, so this is not directly back-portable to 1.2.x. If there is interest, I can create a IE8 compatible version (using `for-loops` etc).}

gkalpak · 2014-10-23T10:28:20Z

Some considerations:

Should we mention in the docs that deep objects can be also filtered (or is it redundant) ?
Should there be inline comments at certain parts explaining that whats and the whys (or is it clear enough).
None of the previous test-cases broke. Yet, since the filter was heavily refactored, one can't be really sure that all corner-cases are handled the same as before.
(The documented stuff does seem to still work as expected, so this shouldn't introduce a breaking change (in the "formal" sense).)

Narretz · 2014-10-23T17:46:02Z

Great work!
So the fix is for with deep expression objects with multiple properties and the feat is that these objects can be nested arbitrarily deep?

I don't want to piggyback on the PR or anything, but can you check if this PR can be incorporated in your fix? It seems like a good fit. #6623

We could also check other filter issues for possible tests to add.

caitp · 2014-10-23T18:46:43Z

stop removing these =)

caitp · 2014-10-23T18:57:51Z

So yeah, I would just ditch the first commit --- 2 lines between specs are nice, and used in most of the test suite --- removing them just makes it more inconsistent. As for the minor whitespace changes, those should be picked up by jscs once we get it doing that, but who really cares if we're honest?

gkalpak · 2014-10-23T19:55:06Z

@caitp:
I am totally OK with two lines between specs as I am with one line.
What I can't stand is inconsistency (2 lines between some specs and 1 line between others). I just removed 3 double-lines to make it consistent across the test-suite (which mostly uses 1 line between specs).

I also removed empty new lines before a closing }); in 3 cases to make it consistent across the test-suite (which also mostly doesn't have empty lines before closing });).

Anyway, those changes (along with a few white-space changes, to make things consistent and inline with the new jscs rules) are placed in a separate commit, so they can't be ignored (only...you know...they really shouldn't, because consistency is a good thing).
If it makes you feel better, I can add double-lines between all specs in the test-suite :)

caitp · 2014-10-23T19:57:42Z

these are test cases, consistency for minor things like that doesn't matter aw hole lot, so long as they are at least following the formal style guidelines imo. I suggest opening a separate bug for fixing style nits unrelated to this functional change so that it's easier to review (less gunk in the diff)

gkalpak · 2014-10-23T20:18:04Z

@caitp: Hm...I thought putting those changes in a separate commit was enough, but it apparently isn't. I'll remove the commit and put it in a separate PR then.

@Narretz: I am not sure if it's a fix or a feat. It depends on how it was supposed to work. If filterFilter was supposed to handle deep expressions (and filter deep objects) then it's a fix. If it wasn't supposed to support that in the first place then it's a feat :)
Will look into other filterFilter related issues, but I can't be sure which ones you want to get in and which you don't. If you can point out PRs that should get merged, I will gladly "adapt" them to feet the refactored filterFilter.

Regarding #6623:
It sure could be incorporated, but I want to point out that:
Right now, it is possible to use {somekey: undefined} to match objects that do not have a somekey property (or have it with a value of undefined). If we merge #6623 this will be only possible using a custom function. So, we might be removing a feature :)
But, if you think it should get merged, I am totally OK with that as well.

caitp · 2014-10-24T18:52:57Z

@gkalpak you probably need to rebase now that the other part is merged. yeah you do

Narretz · 2014-10-24T20:29:55Z

@gkalpak Regarding #6623; didn't think about that, good point. Leave it as is. :)

…prop Basically, implement the logic detailed in the 2nd point of angular#9757 (comment)

Previously, trying to use a deep expression object (i.e. an object whose properties can be objects themselves) did not work correctly. This commit refactors `filterFilter`, making it simpler and adding support for filtering collections of arbitrarily deep objects. Closes angular#7323 Closes angular#9698

Related to angular#9984

…ed properties Closes angular#9984

…prop Basically, implement the logic detailed in the 2nd point of angular#9757 (comment)

caitp · 2014-12-02T17:25:35Z

So I know I threatened to land this a few weeks ago --- I think we want this, but it looks like some stuff has been added since, and it's really hard to keep track of this diff u_u

Generally I trust @gkalpak's judgement though, so it's probably good. @petebacondarwin can we land this today?

petebacondarwin · 2014-12-02T18:43:42Z

It's a big diff but I like that there are only additions in the test file and this is not the most central part of Angular. Let's get this in.

caitp · 2014-12-02T18:51:37Z

Alright *fingers crossed no regressions*

caitp · 2014-12-02T18:54:23Z

IT IS DONE ✨

…prop Basically, implement the logic detailed in the 2nd point of #9757 (comment)

gkalpak · 2014-12-02T20:12:07Z

✨ ✨ OMG ✨ ✨

I think I'm gonna 😂

mary-poppins added cla: yes and removed cla: no labels Oct 23, 2014

gkalpak force-pushed the filterFilter-deep-expr-obj branch 3 times, most recently from 3b040cd to b72583f Compare October 23, 2014 12:20

caitp reviewed Oct 23, 2014
View reviewed changes

Comment thread test/ng/filter/filterSpec.js Outdated

caitp Oct 23, 2014

Copy link
Copy Markdown

Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

stop removing these =)

caitp added type: bug type: refactor component: filters frequency: low frequency: moderate severity: broken expected use and removed frequency: low labels Oct 23, 2014

caitp added this to the 1.3.x milestone Oct 23, 2014

caitp self-assigned this Oct 23, 2014

gkalpak force-pushed the filterFilter-deep-expr-obj branch 4 times, most recently from 0d9a987 to d412eab Compare October 24, 2014 06:26

gkalpak force-pushed the filterFilter-deep-expr-obj branch from d412eab to 7bd2a58 Compare October 25, 2014 05:13

lgalfaso modified the milestones: 1.3.5, 1.3.4 Nov 25, 2014

gkalpak force-pushed the filterFilter-deep-expr-obj branch from 945d75a to 122c228 Compare November 27, 2014 09:16

gkalpak added a commit to gkalpak/angular.js that referenced this pull request Nov 27, 2014

fix(filterFilter): don't match primitive sub-expressions against any …

122c228

…prop Basically, implement the logic detailed in the 2nd point of angular#9757 (comment)

gkalpak force-pushed the filterFilter-deep-expr-obj branch from 122c228 to 2664892 Compare December 1, 2014 09:13

gkalpak added a commit to gkalpak/angular.js that referenced this pull request Dec 1, 2014

fix(filterFilter): don't match primitive sub-expressions against any …

2664892

…prop Basically, implement the logic detailed in the 2nd point of angular#9757 (comment)

gkalpak force-pushed the filterFilter-deep-expr-obj branch from 2664892 to d27f4c5 Compare December 1, 2014 13:15

gkalpak added a commit to gkalpak/angular.js that referenced this pull request Dec 1, 2014

fix(filterFilter): don't match primitive sub-expressions against any …

d27f4c5

…prop Basically, implement the logic detailed in the 2nd point of angular#9757 (comment)

pkozlowski-opensource modified the milestones: 1.3.5, 1.3.6 Dec 1, 2014

gkalpak added 4 commits December 2, 2014 16:29

test(filter): test expression object with inherited properties

d9b710a

Related to angular#9984

fix(filterFilter): ignore function properties and account for inherit…

999ab01

…ed properties Closes angular#9984

fix(filterFilter): don't match primitive sub-expressions against any …

5d40a89

…prop Basically, implement the logic detailed in the 2nd point of angular#9757 (comment)

gkalpak force-pushed the filterFilter-deep-expr-obj branch from d27f4c5 to 5d40a89 Compare December 2, 2014 14:30

gkalpak closed this in f7cf846 Dec 2, 2014

gkalpak added a commit that referenced this pull request Dec 2, 2014

fix(filterFilter): don't match primitive sub-expressions against any …

a75537d

…prop Basically, implement the logic detailed in the 2nd point of #9757 (comment)

albertboada mentioned this pull request Dec 3, 2014

fix(filterFilter) deeply-nested predicate objects + multiple conditions were not giving expected results #7329

Closed

gkalpak mentioned this pull request Dec 9, 2014

fix(filterFilter): comparator should return true if deep object key is undefined #6623

Closed

shuhei mentioned this pull request Dec 10, 2014

fix(filterFilter): filter deep object by string #10401

Closed

This was referenced Dec 11, 2014

fix(filterFilter): make $ match properties on deeper levels as well #10408

Closed

Filter behavior change in 1.3.6 #10419

Closed

ng-repeat filter behavior #10428

Closed

Conversation

gkalpak commented Oct 23, 2014

Uh oh!

gkalpak commented Oct 23, 2014

Uh oh!

Narretz commented Oct 23, 2014

Uh oh!

caitp Oct 23, 2014

Choose a reason for hiding this comment

Uh oh!

caitp commented Oct 23, 2014

Uh oh!

gkalpak commented Oct 23, 2014

Uh oh!

caitp commented Oct 23, 2014

Uh oh!

gkalpak commented Oct 23, 2014

Uh oh!

caitp commented Oct 24, 2014

Uh oh!

Narretz commented Oct 24, 2014

Uh oh!

caitp commented Dec 2, 2014

Uh oh!

petebacondarwin commented Dec 2, 2014

Uh oh!

caitp commented Dec 2, 2014

Uh oh!

caitp commented Dec 2, 2014

Uh oh!

gkalpak commented Dec 2, 2014

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants